Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block consensus methods refactoring #3571

Merged
merged 47 commits into from
Aug 13, 2024

Conversation

scorbajio
Copy link
Contributor

@scorbajio scorbajio commented Aug 7, 2024

This change reforms certain consensus-related Block and BlockHeader class methods into standalone functions, as proposed in #3563.

Copy link

codecov bot commented Aug 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.83%. Comparing base (4470cc3) to head (831e1d6).
Report is 2 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block ?
client ?
tx 77.77% <ø> (ø)
wallet 0.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@scorbajio
Copy link
Contributor Author

scorbajio commented Aug 7, 2024

Testing the bundle size with a standard code path:

import { Block } from '@ethereumjs/block'
import { Chain, Common, Hardfork } from '@ethereumjs/common'
const common = new Common({ chain: Chain.Mainnet, hardfork: Hardfork.London })

const block = Block.fromBlockData(
  {
    header: {
      baseFeePerGas: BigInt(10),
      gasLimit: BigInt(100),
      gasUsed: BigInt(60),
    },
  },
  { common }
)

console.log(Number(block.header.calcNextBaseFee())) // 11

before: 325.8kb
after: 322.9kb
delta: -2.9kb

@scorbajio
Copy link
Contributor Author

scorbajio commented Aug 7, 2024

Testing the bundle size with a standard code path:

import { Block } from '@ethereumjs/block'
import { Chain, Common, Hardfork } from '@ethereumjs/common'
const common = new Common({ chain: Chain.Mainnet, hardfork: Hardfork.London })

const block = Block.fromBlockData(
  {
    header: {
      baseFeePerGas: BigInt(10),
      gasLimit: BigInt(100),
      gasUsed: BigInt(60),
    },
  },
  { common }
)

console.log(Number(block.header.calcNextBaseFee())) // 11

before: 325.8kb after: 322.9kb delta: -2.9kb

Taking a look at the output bundle, it seems like some of the consensus functions are still being included in the final output bundle, will have to look into why this is happening as the code path doesn't seem to use them.

packages/block/src/consensus/index.ts Show resolved Hide resolved
packages/block/src/header.ts Outdated Show resolved Hide resolved
packages/evm/package.json Outdated Show resolved Hide resolved
packages/evm/src/types.ts Outdated Show resolved Hide resolved
…reumjs/ethereumjs-monorepo into block-consensus-methods-refactoring
@holgerd77
Copy link
Member

Nice, this looks great now! 🤩

@holgerd77
Copy link
Member

(interesting failure in client tests though)

Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@acolytec3 acolytec3 dismissed holgerd77’s stale review August 13, 2024 19:05

Further comments have confirmed that the code is good now.

@acolytec3 acolytec3 merged commit 210b0fa into master Aug 13, 2024
34 checks passed
@acolytec3 acolytec3 deleted the block-consensus-methods-refactoring branch August 13, 2024 19:05
@holgerd77
Copy link
Member

Ah, looking through this I have to say, this feels really good to have this consensus code and this specific methods out of core Block, also changes all look good, had a final look over the code changes! Thanks Amir, nice work! 👍 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block -> Tree Shaking: Separate Consensus Methods
4 participants